#6057 Improve startup time #7486

Ali96kz · 2021-03-04T18:57:32Z

#6057 Check string is number with regex instead of throwing exception. It will improve start time because when I have 3000+ entries it throws about ~170.000 NumberFormatException

Change in CHANGELOG.md described in a way that is understandable for the average user (if applicable)
Tests created for changes (if applicable)
Manually tested changed features in running JabRef (always required)
Screenshots added in PR description (for UI changes)
Checked documentation: Is the information available and up to date? If not created an issue at https://github.com/JabRef/user-documentation/issues or, even better, submitted a pull request to the documentation repository.

Siedlerchr · 2021-03-04T19:08:44Z

I like the idea, but please check the org.jabref.gui.util.comparator.NumericFieldComparatorTest

tobiasdiez · 2021-03-04T22:48:37Z

Thanks, that's indeed a nice improvement. However, regex are also pretty expensive, so there is still room for improvement. For example, https://stackoverflow.com/questions/11875424/java-regular-expression-offers-any-performance-benefit discusses a neat way using character-wise checks. There are some issues with this approach e.g. with negative or decimal numbers, but that's not really something we care about in the main table, so we can ignore them.

Another huge improvement would be to not use the numeric comparator for all fields by default. For example, it doesn't make any sense to try comparing authors by converting them first to numbers. I think, only the year columns needs such a treatment...and custom columns for which we don't know the format #6349.

Ali96kz · 2021-03-05T19:12:51Z

@tobiasdiez I changed the number validation mechanism. I don't know how to implement the second suggestion because there is no way to get data type from MainTableColumnModel, what about creating a separate task?

tobiasdiez

Thanks for the quick follow-up. Two minor remarks from my side.

Concerning the installation of the compartor in the main table, you have the field available at

jabref/src/main/java/org/jabref/gui/maintable/columns/FieldColumn.java

Line 21 in c8f7be8

this.fields = FieldFactory.parseOrFields(model.getQualifier());

.
Thus you could add a check at

jabref/src/main/java/org/jabref/gui/maintable/columns/FieldColumn.java

Line 29 in c8f7be8

this.setComparator(new NumericFieldComparator());

if the field is a UnknownField or if it is numeric:

jabref/src/main/java/org/jabref/model/entry/field/Field.java

Line 35 in f7de0a6

default boolean isNumeric() {

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java

tobiasdiez

The checkstyle fails; apart from this is good to go from my side.

Ali96kz · 2021-03-09T06:56:27Z

@k3KAW8Pnf7mkmdSMPHz27 Are you a second reviewer?

k3KAW8Pnf7mkmdSMPHz27 · 2021-03-09T12:22:14Z

@Ali96kz unless someone else is taking a look at it earlier, I should be able to review it in a couple of hours.

k3KAW8Pnf7mkmdSMPHz27

It looks good to me! There is only one change I truly require (line 51 in NumericFieldComparator) and the rest are perhaps nitpicking.
Other than that, good changes. I am looking forward to the performance improvement 😍

(Sorry about the slightly later than promised review, I were less familiar with OrFields than I expected)

k3KAW8Pnf7mkmdSMPHz27 · 2021-03-09T17:02:15Z

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java

-        } catch (NumberFormatException ignore) {
-            // do nothing
+        // If we arrive at this stage then both values are actually numeric !
+        return valInt1 - valInt2;


I know this isn't your code, but I'd rather see valInt1.compareTo(valInt2) here (even if it should not overflow with reasonable values). I believe it will help with avoiding auto-(un)boxing as well.

It will break a test

For compareTwoNumericInputs I'd go either with 1 or changing it along the lines of https://junit.org/junit5/docs/snapshot/user-guide/index.html#writing-tests-test-interfaces-and-default-methods
e.g.,

assertTrue(comparator.compare("4", "2") > 0);

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java

src/test/java/org/jabref/gui/util/comparator/NumericFieldComparatorTest.java

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java

Siedlerchr

thanks for tackling this issue!

check with regex instead of throwing exception

fc844dd

fix NumericFieldComparatorTest

adbfb37

Ali96kz added 2 commits March 5, 2021 21:55

change number validation method

aa233e6

add changes in CHANGELOG.md

5a54c7e

fix fails on empty string

06f2662

Siedlerchr added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Mar 6, 2021

add case when only '-'

d78d9b7

tobiasdiez reviewed Mar 7, 2021

View reviewed changes

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java Outdated Show resolved Hide resolved

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java Outdated Show resolved Hide resolved

Ali96kz added 2 commits March 8, 2021 14:45

add NumericFieldComparator for number column only

aec877c

add case for '+' sign

ef3487f

tobiasdiez approved these changes Mar 8, 2021

View reviewed changes

fix checkstyle

c6b5d9f

k3KAW8Pnf7mkmdSMPHz27 requested changes Mar 9, 2021

View reviewed changes

add corner case with '+'

d9da965

k3KAW8Pnf7mkmdSMPHz27 reviewed Mar 9, 2021

View reviewed changes

src/main/java/org/jabref/gui/util/comparator/NumericFieldComparator.java Outdated Show resolved Hide resolved

add corner case with '+'

73c2323

k3KAW8Pnf7mkmdSMPHz27 approved these changes Mar 9, 2021

View reviewed changes

Siedlerchr approved these changes Mar 10, 2021

View reviewed changes

Siedlerchr merged commit 50ba021 into JabRef:master Mar 10, 2021

Ali96kz mentioned this pull request Mar 30, 2021

Performance issues with version 5.0 (example videos) #5071

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#6057 Improve startup time #7486

#6057 Improve startup time #7486

Ali96kz commented Mar 4, 2021 •

edited

Loading

Siedlerchr commented Mar 4, 2021

tobiasdiez commented Mar 4, 2021

Ali96kz commented Mar 5, 2021 •

edited

Loading

tobiasdiez left a comment

tobiasdiez left a comment

Ali96kz commented Mar 9, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Mar 9, 2021

k3KAW8Pnf7mkmdSMPHz27 left a comment •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 Mar 9, 2021

Ali96kz Mar 9, 2021

k3KAW8Pnf7mkmdSMPHz27 Mar 9, 2021

Siedlerchr left a comment

#6057 Improve startup time #7486

#6057 Improve startup time #7486

Conversation

Ali96kz commented Mar 4, 2021 • edited Loading

Siedlerchr commented Mar 4, 2021

tobiasdiez commented Mar 4, 2021

Ali96kz commented Mar 5, 2021 • edited Loading

tobiasdiez left a comment

Choose a reason for hiding this comment

tobiasdiez left a comment

Choose a reason for hiding this comment

Ali96kz commented Mar 9, 2021

k3KAW8Pnf7mkmdSMPHz27 commented Mar 9, 2021

k3KAW8Pnf7mkmdSMPHz27 left a comment • edited Loading

Choose a reason for hiding this comment

k3KAW8Pnf7mkmdSMPHz27 Mar 9, 2021

Choose a reason for hiding this comment

Ali96kz Mar 9, 2021

Choose a reason for hiding this comment

k3KAW8Pnf7mkmdSMPHz27 Mar 9, 2021

Choose a reason for hiding this comment

Siedlerchr left a comment

Choose a reason for hiding this comment

Ali96kz commented Mar 4, 2021 •

edited

Loading

Ali96kz commented Mar 5, 2021 •

edited

Loading

k3KAW8Pnf7mkmdSMPHz27 left a comment •

edited

Loading